-
Notifications
You must be signed in to change notification settings - Fork 231
feat(webvh): Added did:webvh resolver #2238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(webvh): Added did:webvh resolver #2238
Conversation
🦋 Changeset detectedLatest commit: 884d3af The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new approach is awesome, thanks @brianorwhatever. I left some comments. It seems some parts are todo, do you want to address these as part of this PR, or in another one?
}, | ||
} | ||
} | ||
const privateKey = Buffer.from(MultiBaseEncoder.decode(privateKeyMultibase).data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to pass an existing public key, but not the private key? We usually create a key first, then use that to create a diddoc
(This is fine to add later, just trying to understand how it works)
throw new Error('Not implemented') | ||
} | ||
|
||
public async verify(signature: Uint8Array, message: Uint8Array, publicKey: Uint8Array): Promise<boolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably good if the interface from did web vh passes the keytype as well, so it's ready to support additional key types as well
export class DIDWebvhCrypto extends AbstractCrypto { | ||
private agentContext?: AgentContext | ||
|
||
public constructor(agentContext?: AgentContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can make agent context required
@TimoGlastra I forgot to mention I was focussing solely on resolver in this PR. I added some of the base stuff for the registrar, which will be coming soon. I'm happy to either remove that stuff or fix it up later. Higher priority is resolving DID documents and resources. |
Ah that makes sense @brianorwhatever. Let's focus this pr on the resolving in that case and we can add the registration parts next 👍 |
Curious is the implementation backwards compatible with older versions? The swiss eid public beta that was announced yesterday uses version 0.3. I'm trying to understand if we could use this resolver / didwebvh-ts for integrating with this? |
OK, great I will remove the stuff about the registrar. Unfortunately, I don't think that this is 0.3 compatible I can compare the differences and see what would be required to make it work. |
Hey @TimoGlastra, quick question about verifying Data Integrity proofs in a custom setup. We're building an Inside our registry logic (in a helper
The last piece is actually verifying the We also considered using the signature suites directly (like Is there a recommended/simpler way in Credo to verify a DI proof on a custom JSON-LD object like this, making sure key resolution via the Currently have it stubbed out, but want to make sure we implement the crypto verification correctly using Credo's infrastructure. Thanks! |
8d90812
to
facc66b
Compare
8acc23d
to
0db8966
Compare
@TimoGlastra @genaris this is ready for a review. I have updated the didwebvh-ts package to the new version which supports version 1.0 of the spec and addressed any comments in this thread. Thanks! |
The integration tests probably don't need to be checked in they are testing real resource resolution and those files will likely disappear one day |
packages/webvh/package.json
Outdated
@@ -0,0 +1,39 @@ | |||
{ | |||
"name": "@credo-ts/webvh", | |||
"version": "0.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be synced with the other packages
"version": "0.4.0", | |
"version": "0.5.13", |
packages/webvh/src/anoncreds/services/WebVhAnonCredsRegistry.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a multihash implementation already: https://github.com/openwallet-foundation/credo-ts/blob/879ed2c9178684d14e6850cbcdb02d6832e0d730/packages/core/src/utils/MultiHashEncoder.ts.
If there's any features missing in our multi hash implementation, it would be better to extend the existing functionaility, this way we prevent duplication of logic
@Transform(({ value }) => { | ||
// Determine which class to use based on the content | ||
if (value && 'attrNames' in value) { | ||
return Object.assign(new WebVhSchemaContent(), value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this stil trigger the validation i you're doing it like this? Usually i use the JsonTransformer, but if this works, it's ok 👍
public readonly allowsCaching = false | ||
public readonly allowsLocalDidRecord = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason these are both false? For the did:web
one they are both true, I'd assume the same values here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was temporary and I forgot to update. true seems fine to me will set them to that.
agentContext.config.logger.debug(`Fetching resource from: ${httpsUrl}`) | ||
|
||
// Fetch the resource using native fetch | ||
const response = await fetch(httpsUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use fetch from the agent config. agentContext.config.agentDependencies.fetch
verificationMethod: { | ||
id: 'did:webvh:123', | ||
controller: 'did:webvh:123', | ||
type: 'Ed25519VerificationKey2020', | ||
publicKeyMultibase: '123', | ||
secretKeyMultibase: '123', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we set it to these sample values? Should this be optional in the AbstractCrypto
?
private async isKmsAvailable(): Promise<boolean> { | ||
try { | ||
return !!(this.agentContext.dependencyManager.resolve(Kms.KeyManagementApi)) | ||
} catch { | ||
return false | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kms will always be available, so i think this check is redundant
} | ||
} | ||
|
||
private async verifyWithLegacyMethod(signature: Uint8Array, message: Uint8Array, publicKey: Uint8Array): Promise<boolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed, if you want this to work with 0.5.x as well, you can make another PR to the 0.5.x
branch based on the old KMS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to an example of the old KMS? I couldn't find any. This was specifically not working in BC Wallet which is on 0.5.13.
Is main
branch for version 0.6.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the main branch is for 0.6.0.
If we want this to be available in both i think the best method is to:
- remove all legacy KMs stuff from this PR
- merge into main
- create a PR to 0.5.x branch, with changes for the legacy KMS
But when targeting the main branch, we should only use the new KMS API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, that makes sense. I'm in the process of removing the legacy stuff and I think I have an idea of how to get 0.5 working. Just created a PR for didwebvh-ts that removes the requirement of passing a VM into the constructor so once that lands I'll have this PR in great shape
packages/webvh/src/anoncreds/services/__tests__/mock-resources.ts
Outdated
Show resolved
Hide resolved
agentContext = getAgentContext({ agentConfig }) | ||
|
||
// Register the mocked resolver instance | ||
agentContext.dependencyManager.registerInstance(WebvhDidResolver, new WebvhDidResolver()) | ||
|
||
// Mock the dependency manager resolve method | ||
const originalResolve = agentContext.dependencyManager.resolve.bind(agentContext.dependencyManager) | ||
agentContext.dependencyManager.resolve = jest.fn().mockImplementation((token) => { | ||
const tokenString = token?.name || token?.toString?.() || String(token) | ||
|
||
if (tokenString.includes('DidsApi')) { | ||
return mockDidsApi | ||
} | ||
if (tokenString.includes('KeyManagementApi')) { | ||
return mockKeyManagementApi | ||
} | ||
if (token === WebvhDidResolver || tokenString.includes('WebvhDidResolver')) { | ||
return { resolveResource: mockResolveResource } | ||
} | ||
return originalResolve(token) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like this would also work:
agentContext = getAgentContext({ agentConfig }) | |
// Register the mocked resolver instance | |
agentContext.dependencyManager.registerInstance(WebvhDidResolver, new WebvhDidResolver()) | |
// Mock the dependency manager resolve method | |
const originalResolve = agentContext.dependencyManager.resolve.bind(agentContext.dependencyManager) | |
agentContext.dependencyManager.resolve = jest.fn().mockImplementation((token) => { | |
const tokenString = token?.name || token?.toString?.() || String(token) | |
if (tokenString.includes('DidsApi')) { | |
return mockDidsApi | |
} | |
if (tokenString.includes('KeyManagementApi')) { | |
return mockKeyManagementApi | |
} | |
if (token === WebvhDidResolver || tokenString.includes('WebvhDidResolver')) { | |
return { resolveResource: mockResolveResource } | |
} | |
return originalResolve(token) | |
}) | |
agentContext = getAgentContext({ | |
agentConfig, | |
registerInstances: [ | |
[DidsApi, mockDidsApi], | |
[KeyManagementApi, mockKeyManagementApi], | |
[WebvhDidResolver, { resolveResource: mockResolveResource }] | |
] | |
}) |
Not sure about this line:
// Register the mocked resolver instance
agentContext.dependencyManager.registerInstance(WebvhDidResolver, new WebvhDidResolver())
As afterwards you overwrite it here:
if (token === WebvhDidResolver || tokenString.includes('WebvhDidResolver')) {
return { resolveResource: mockResolveResource }
}
You can update the entry in registerInstances
for WebvhDidResolver
with the value you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worked great. thanks!
packages/webvh/src/anoncreds/services/WebVhAnonCredsRegistry.ts
Outdated
Show resolved
Hide resolved
Thank you @TimoGlastra I believe I addressed all of your comments other than the legacy KMS stuff. Need a bit more help getting that plugged in properly |
Signed-off-by: Brian Richter <brian@aviary.tech>
- Removed WebvhDidRegistrar class and its related exports from the dids module. - Updated index.ts and WebvhModule.ts to reflect the removal of the registrar. - Adjusted DIDWebvhCrypto class to ensure agentContext is always defined. - Cleaned up unused functions and imports in didWebvhUtil.ts. Signed-off-by: Brian Richter <brian@aviary.tech>
…ial definition support - Added WebVhAnonCredsRegistry class to handle anon credentials with did:webvh identifiers. - Implemented methods for retrieving schemas, credential definitions, and revocation registry definitions. - Introduced validation and error handling for resource resolution and proof verification. - Created utility functions for multihash encoding and base58 conversion. - Added tests for WebVhAnonCredsRegistry and resource transformation. - Updated package.json to include new dependencies for class-transformer and class-validator. Signed-off-by: Brian Richter <brian@aviary.tech>
- Updated valid DID and related references in tests to reflect the proper DID format. - Ensured consistency across the test suite by replacing old example DIDs with the updated structure. Signed-off-by: Brian Richter <brian@aviary.tech>
…itialization - Deleted WebvhModuleConfig class and its related exports to streamline the module configuration. - Updated WebvhModule to remove dependency on WebvhModuleConfig, simplifying its constructor. - Adjusted test setup to reflect the removal of configuration options. Signed-off-by: Brian Richter <brian@aviary.tech>
… method signatures - Removed unused imports and streamlined type definitions for resource resolution results. - Updated method signatures to reflect changes in type definitions, enhancing clarity. - Consolidated register methods to eliminate unnecessary parameters, maintaining consistency in error handling. Signed-off-by: Brian Richter <brian@aviary.tech>
…atibility - Updated 'didwebvh-ts' version to ^2.2.0 in package.json. Signed-off-by: Brian Richter <brian@aviary.tech>
- Updated TypeScript version from 4.9.5 to 5.5.4 in package.json and pnpm-lock.yaml for improved compatibility. - Removed the obsolete tsconfig.test.json file to streamline project structure. - Simplified tsconfig.json by removing unnecessary path mappings. - Adjusted test files to reflect changes in type definitions and imports. Signed-off-by: Brian Richter <brian@aviary.tech>
…nd resource resolution improvements - Implemented a new `verifyProof` method for validating DataIntegrityProofs using eddsa-jcs-2022. - Enhanced error handling in `_resolveAndValidateAttestedResource` for better clarity on resolution failures. - Updated the `WebvhDidResolver` to fetch resources from the specified URLs and handle various content types. - Added comprehensive tests for the new proof verification logic and resource resolution, ensuring robustness and reliability. Signed-off-by: Brian Richter <brian@aviary.tech>
…with improved crypto handling and resource resolution - Introduced `DIDWebvhCrypto` for streamlined signature verification using JWK and KMS. - Updated `WebVhAnonCredsRegistry` to utilize the new crypto class for signature verification. - Enhanced `WebvhDidResolver` with a new `getBaseUrl` method for better DID URL parsing. - Removed obsolete test cases and added integration tests for `WebvhDidResolver` to ensure robust resource resolution. Signed-off-by: Brian Richter <brian@aviary.tech>
…methods - Added '@stablelib/ed25519' dependency for enhanced Ed25519 signature verification. - Updated 'didwebvh-ts' version to ^2.3.2 for improved functionality. - Refactored signature verification in DIDWebvhCrypto to utilize KMS or fallback to legacy methods as needed. Signed-off-by: Brian Richter <brian@aviary.tech>
- Added MultiBaseEncoder and MultiHashEncoder to improve encoding and decoding processes. - Updated WebVhAnonCredsRegistry to utilize new encoding methods for hash generation. - Refactored utility functions to remove deprecated base58 and multihash implementations. - Enhanced error logging in WebVhAnonCredsRegistry for better clarity on issues. - Updated tests to reflect changes in resource handling and encoding logic. Signed-off-by: Brian Richter <brian@aviary.tech>
- Added export for WebVhAnonCredsRegistry in index.ts to enhance module accessibility. Signed-off-by: Brian Richter <brian@aviary.tech>
4b18dec
to
884d3af
Compare
"dependencies": { | ||
"@credo-ts/anoncreds": "workspace:*", | ||
"@credo-ts/core": "workspace:*", | ||
"@multiformats/base-x": "^4.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be removed now?
"@credo-ts/anoncreds": "workspace:*", | ||
"@credo-ts/core": "workspace:*", | ||
"@multiformats/base-x": "^4.0.1", | ||
"@stablelib/ed25519": "^2.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one also
For the legacy KMS, we also supported Ed25519 ,so it should not be needed to use this (and we prefer doing everything through the pluggable crypto interface)
if (error instanceof CredoError) { | ||
if (error.message.includes('parse resource data')) errorType = 'invalidJson' | ||
else if (error.message.includes('resolve') || error.message.includes('missing data')) errorType = 'notFound' | ||
else if (error.message.includes('attested')) errorType = 'invalid' | ||
else if (error.message.includes('proof')) errorType = 'invalid' | ||
else if (error.message.includes('hash mismatch')) errorType = 'invalid' | ||
else if (error.message.includes('not a valid schema')) errorType = 'invalid' | ||
else if (error.message.includes('Invalid schema ID')) errorType = 'invalidDid' | ||
else if (error.message.includes('Issuer ID mismatch')) errorType = 'invalid' | ||
else if (error.message.includes('missing a valid issuerId')) errorType = 'invalid' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these error messsages constructed? We don't use error messages anywhere to handle errors as it's very brittle. If you want to catch these cases, best i think is to create and throw specific errors that you can check with instanceof
@genaris @TimoGlastra as discussed I have pulled the crypto out of didwebvh-ts while also removing all of the dependencies. Please take a look at this new PR